fix: make FIP-0115 activation height configurable#6830
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors FIP-0115 activation to be environment-configurable via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
57e459d to
443a849
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/docs/users/reference/env_variables.md (1)
67-67: Consider adding a Lotus-interop note for operators.Optional: mention the Lotus equivalent (
LOTUS_FEES_FIP0115HEIGHT) to reduce migration confusion in mixed deployments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/users/reference/env_variables.md` at line 67, Add a brief Lotus-interop note alongside the FOREST_FEES_FIP0115HEIGHT entry clarifying the equivalent Lotus environment variable LOTUS_FEES_FIP0115HEIGHT so operators migrating or running mixed deployments understand the mapping; update the table cell or a short footnote to mention "Lotus equivalent: LOTUS_FEES_FIP0115HEIGHT" and indicate it has the same semantics (integer epoch, -1 to disable, consensus-breaking/testing only).src/chain/store/base_fee.rs (1)
20-21: Add explicit runtime warning when the consensus override is enabled.
FOREST_FEES_FIP0115HEIGHTis consensus-breaking, but activation is currently silent. A one-time warning here would reduce operator misconfiguration risk.🔧 Suggested change
use std::sync::LazyLock; +use tracing::warn; static FIP0115_HEIGHT: LazyLock<ChainEpoch> = - LazyLock::new(|| env_or_default("FOREST_FEES_FIP0115HEIGHT", -1)); + LazyLock::new(|| { + let height = env_or_default("FOREST_FEES_FIP0115HEIGHT", -1); + if height >= 0 { + warn!( + "FOREST_FEES_FIP0115HEIGHT={} enabled; this is consensus-breaking and intended for testing only", + height + ); + } + height + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/base_fee.rs` around lines 20 - 21, FIP0115_HEIGHT is being set silently from env_or_default("FOREST_FEES_FIP0115HEIGHT", -1); change the LazyLock initialization so that when the resolved value is not the default (-1) it emits a one-time runtime warning (e.g., via log::warn! or the project logger) informing operators that a consensus override is enabled and which epoch value is set; keep the value semantics the same but ensure the warning happens inside the LazyLock::new closure for FIP0115_HEIGHT so it runs once at startup when the env var is active.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/docs/users/reference/env_variables.md`:
- Line 67: Add a brief Lotus-interop note alongside the
FOREST_FEES_FIP0115HEIGHT entry clarifying the equivalent Lotus environment
variable LOTUS_FEES_FIP0115HEIGHT so operators migrating or running mixed
deployments understand the mapping; update the table cell or a short footnote to
mention "Lotus equivalent: LOTUS_FEES_FIP0115HEIGHT" and indicate it has the
same semantics (integer epoch, -1 to disable, consensus-breaking/testing only).
In `@src/chain/store/base_fee.rs`:
- Around line 20-21: FIP0115_HEIGHT is being set silently from
env_or_default("FOREST_FEES_FIP0115HEIGHT", -1); change the LazyLock
initialization so that when the resolved value is not the default (-1) it emits
a one-time runtime warning (e.g., via log::warn! or the project logger)
informing operators that a consensus override is enabled and which epoch value
is set; keep the value semantics the same but ensure the warning happens inside
the LazyLock::new closure for FIP0115_HEIGHT so it runs once at startup when the
env var is active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 710e2507-13d3-46e2-9c46-450acde26cd9
📒 Files selected for processing (6)
CHANGELOG.mddocs/docs/users/reference/env_variables.mdsrc/chain/store/base_fee.rssrc/chain_sync/tipset_syncer.rssrc/message_pool/msgpool/provider.rssrc/rpc/methods/miner.rs
💤 Files with no reviewable changes (1)
- src/rpc/methods/miner.rs
443a849 to
54a9c3c
Compare
|
@LesnyRumcajs Also there is one suggestion by coderabbit, if you think it's worth it:
|
Summary of changes
Changes introduced in this pull request:
LOTUS_FEES_FIP0115HEIGHT) filecoin-project/lotus#13550 to ForestReference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Documentation